Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the configerator config source doc. #1190

Merged
merged 3 commits into from
Dec 11, 2020

Conversation

jieru-hu
Copy link
Contributor

@jieru-hu jieru-hu commented Dec 9, 2020

Motivation

Move the doc to hydra doc.

Have you read the Contributing Guidelines on pull requests?

Yes/No

Test Plan

Build it locally, verified it looked ok

Screen Shot 2020-12-08 at 6 27 28 PM
image

Related Issues and PRs

(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)

@jieru-hu jieru-hu requested a review from omry December 9, 2020 02:30
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 9, 2020
@omry
Copy link
Collaborator

omry commented Dec 10, 2020

Cool.
I think this plugin is not as useful as it can be before we have support for configuring the config searchpath.
I don't mind landing this though but I think we should call out that things will be easier once #274 is done.
By the way: there is a chicken and egg here:
The primary config cannot be in configerator if we support that unless we also support arbitrary searchpath urls in @hydra.main():

@hydra.main(config_path="configerator://my_project", config_name="config")
def main(cfg):
  ...

website/docs/fb/configerator-config-source.md Outdated Show resolved Hide resolved
website/docs/fb/configerator-config-source.md Outdated Show resolved Hide resolved
@jieru-hu
Copy link
Contributor Author

Cool.
I think this plugin is not as useful as it can be before we have support for configuring the config searchpath.
I don't mind landing this though but I think we should call out that things will be easier once #274 is done.
By the way: there is a chicken and egg here:
The primary config cannot be in configerator if we support that unless we also support arbitrary searchpath urls in @hydra.main():

@hydra.main(config_path="configerator://my_project", config_name="config")
def main(cfg):
  ...

I'm not sure I get this one. We can still read from configerator as primary if the SearchPathPlugin is defined and the config name exists in configerator. (and I've tested that in fbcode it works. ) maybe we can chat about this in person tomorrow.

@jieru-hu jieru-hu requested a review from omry December 10, 2020 23:17
@omry
Copy link
Collaborator

omry commented Dec 11, 2020

I'm not sure I get this one. We can still read from configerator as primary if the SearchPathPlugin is defined and the config name exists in configerator. (and I've tested that in fbcode it works. ) maybe we can chat about this in person tomorrow.

I am talking about the situation when we no longer need the SearchPathPlugin because we have the ability to configure the searchpath in the primary config.

Copy link
Collaborator

@omry omry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@jieru-hu
Copy link
Contributor Author

I'm not sure I get this one. We can still read from configerator as primary if the SearchPathPlugin is defined and the config name exists in configerator. (and I've tested that in fbcode it works. ) maybe we can chat about this in person tomorrow.

I am talking about the situation when we no longer need the SearchPathPlugin because we have the ability to configure the searchpath in the primary config.

ah got it! thx for the clarification.

@jieru-hu jieru-hu merged commit 522431d into facebookresearch:master Dec 11, 2020
@jieru-hu jieru-hu deleted the configerator branch December 11, 2020 03:24
jieru-hu added a commit to jieru-hu/hydra that referenced this pull request Dec 11, 2020
jieru-hu added a commit that referenced this pull request Dec 11, 2020
* Add configerator config source doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants